Skip to content

build: enable Spark SQL tests for released Spark 4.2.0 - #4950

Open
andygrove wants to merge 43 commits into
apache:mainfrom
andygrove:spark-4.2.0
Open

andygrove wants to merge 43 commits into
apache:mainfrom
andygrove:spark-4.2.0

Conversation

@andygrove

@andygrove andygrove commented Jul 16, 2026 •

Copy link
Copy Markdown
Member

Which issue does this PR close?

Part of #4142. Also narrows the Spark 4.2 half of #5571.

Supersedes #4208, which was opened against 4.2.0-preview4 and went stale.

Rationale for this change

Spark 4.2.0 is now available in Maven Central. This is stage 3 of the bring-up described in adding_a_new_spark_version.md: turn on Spark's own SQL tests for 4.2. The profile bump to the released 4.2.0, the CometInternalRowShim split it forced, and the regenerated plan stability goldens went in separately as #4960, so this PR is now only the Spark SQL test diff, the CI wiring, and the Comet bugs those test runs exposed.

Spark 4.2.0 is not simply 4.2.0-preview4 with the qualifier dropped. It restructures the test harness and adds FILTER (WHERE ...) support for window aggregates, so several real reconciliations were needed.

What changes are included in this PR?

Comet fixes exposed by the 4.2 test runs

  • CometNativeScan rejects a scan whose data or partition schema contains a type with no proto representation (GEOMETRY / GEOGRAPHY on 4.2), which otherwise threw NoSuchElementException during planning instead of falling back. The native scan serializes the full data schema, not just the projected columns, so the check cannot be limited to the required schema. CometScanRule now runs the projected/partition schema type check before this one so that a column whose type is unsupported is still reported as such (Unsupported s of type VariantType) rather than as the coarser serialization failure. New fixture expressions/misc/geospatial_types.sql covers the GEOMETRY / GEOGRAPHY fallback, including the case where the geospatial column is not projected.
  • Spark 4.2 allows FILTER (WHERE ...) on a window aggregate. Comet dropped the filter silently -- aggExprToProto only serialized it for Partial mode aggregates and window aggregates are Complete mode -- and DataFusion window expressions have no filter support anyway, so window.sql got wrong results. CometWindowExec now declines window expressions whose aggregate carries a filter, and aggExprToProto falls back instead of silently dropping a filter for any non-Partial mode. New fixture expressions/window/window_filter.sql fails (wrong results) without the fix.

Spark SQL test diff

  • Adds dev/diffs/4.2.0.diff, seeded from the current 4.1.3.diff against the v4.2.0 tag. (chore: bump spark-4.2 profile to the released 4.2.0 #4960 already deleted the stale 4.2.0-preview4.diff.)
  • The main reconciliation is upstream's test-harness refactor: SQLTestUtils shrinks to a deprecated empty alias, and withSQLConf, stripSparkFilter and the test() override move into QueryTest, with SharedSparkSession now extending QueryTest directly. Comet's hooks (isCometEnabled, the IgnoreComet skip, and the stripSparkFilter Comet cases) move to QueryTest accordingly. isCometEnabled must qualify classic.SparkSession, since a bare SparkSession in that package resolves to the unified class.
  • KeyGroupedPartitioningSuite needs no Comet change any more: 4.2 already declares collectAllShuffles / collectShuffles as protected returning Seq[ShuffleExchangeLike].
  • HiveUDFDynamicLoadSuite drops Comet's commented-out assume: 4.2 adds TestHiveUdfsJar, which builds hive-test-udfs.jar from Java sources at runtime, so the stripped-jar workaround is obsolete.
  • Two 4.2-only fixtures run with --SET spark.comet.enabled = false, following the existing precedents in the diff:
    • join-nearest-by.sql: its EXPLAIN queries record Spark's operator names in the golden file (same treatment as explain.sql / explain-aqe.sql / explain-cbo.sql).
    • subquery/in-subquery/in-order-by.sql: one query sorts on a column with tied keys, so the order of the tied rows is implementation defined and Comet's sort does not reproduce the order recorded in the golden file (same treatment as in-limit.sql).
  • The invalid-UTF-8 sketch fixtures (hll.sql, thetasketch.sql and the 4.2-only tuplesketch.sql) run with Comet. One table in each holds invalid UTF-8, which Spark allows in a STRING column and Comet's native scan rejects (Comet native scan rejects invalid UTF-8 byte sequences in STRING column (hll.sql on Spark 4.1) #4121). So SQLQueryTestSuite disables only the native scan (spark.comet.scan.enabled), and only for the queries that name hll_string_test or t_string_collation: the eight SELECTs per file that read those tables, 24 of the 454 statements in the three files. The fixtures themselves are not patched and no golden file changes. ignoreList has no Comet additions.

CI

  • Adds a spark_4_2 job to the ci.yml umbrella calling spark_sql_test_reusable.yml (Spark 4.2.0, JDK 17), plus the matching spark_4_2 filter in compute-changes.py and output on the changes job. The job sits in the nightly tier, alongside Spark 3.5 and 4.0, with run-spark-4.2-tests to bring the run forward onto a pull request. It gates no merge: a red nightly opens a ci-nightly-failure issue rather than blocking the queue.
  • Nightly rather than on demand because of what on demand costs the diff file. Every other version's dev/diffs entry is updated as a side effect of its suite running; an on-demand 4.2 would be updated only when someone asked for the suite. That is not hypothetical -- see the diff resync below.
  • Fixes a merge hazard the new job inherits: ci: bootstrap Maven from the setup actions so every mvnw job is covered #5881 added .github/actions/maven-bootstrap/** to every Spark SQL change filter, and because spark_4_2 exists only on this branch that entry was appended to five sibling lists and never grew a sixth. check-ci-config.py fails on that, which would take preflight down.

Docs

  • The experimental table in the user guide and the compatibility page both record the 4.2 Spark SQL tests as running in CI; the compatibility warning says they run nightly rather than on every pull request. ci.md and spark-sql-tests.md list 4.2 among the nightly-tier versions.
  • The new-Spark-version contributor guide described the spark_sql_test.yml matrix, which has since been replaced by the per-version umbrella jobs; it now documents the ci.yml + compute-changes.py wiring and the staging a new version goes through. It also says explicitly not to linger on demand once the suite passes, and why: the diff rots.
  • The same guide now says to build a new version's FILTERS list by copying the nearest version's whole list, since a dropped shared .github/actions/** path makes the job skip precisely when the input it needed changed.

How are these changes tested?

  • -Pspark-4.2, the default profile (4.1) and -Pspark-3.4 -Pscala-2.12 all build main and test sources clean; scalastyle and spotless run in the same phase.
  • dev/diffs/4.2.0.diff was verified to apply with no rejects to a pristine v4.2.0 checkout, and patches the same files as 4.1.3.diff apart from the expected SQLTestUtils to QueryTest swap, the 4.2-only suites, and the two fixture files above.
  • CometSqlFileTestSuite passes under -Pspark-4.2, -Pspark-4.1, -Pspark-4.0 and -Pspark-3.5, covering the two new fixtures (skipped below 4.2 via MinSparkVersion) and the changed scan fallback reasons.
  • CometTPCDSV1_4_PlanStabilitySuite and CometTPCDSV2_7_PlanStabilitySuite pass under -Pspark-4.2 (97 and 32 tests, no failures).
  • The full Spark 4.2 SQL test run was exercised on this PR at commit 65d16df: all seven shards (catalyst, sql_core-1/2/3, sql_hive-1/2/3) passed.
  • Re-verified after merging current main, which had meanwhile changed the shared q78 golden (round now goes through the codegen dispatcher): both plan stability suites, both new fixtures, and CometNativeScanSuite / CometVariantTypeSuite / CometExecRuleSuite pass under -Pspark-4.2, and the same three suites pass under the default profile. The merge had one conflict, in CometNativeScan: main inlined the Variant data-schema pruning that this branch had factored into a helper for reuse by isSupported, so the helper is kept and the inline copy dropped.

Second merge of main

Merging main again, and scoping the sketch opt-out (this update)

  • Two more merges, up to b01c168, each with a conflict:
  • 4.2.0.diff resync, regenerated from a v4.2.0 tree as before: it drops spark.comet.memoryOverhead (feat: remove memory accounting from Comet's on-heap mode #6066). Two other sibling-diff changes are deliberately not ported. test: accept CometHashAggregateExec in the Spark 4.0 CollationSuite hash agg check #6220's CollationSuite change was already in the 4.2 patch. feat: support direct Variant projection in native Parquet scans #5868's ResolveDefaultColumnsSuite edit to 4.1.3.diff would weaken a test that passes unchanged on 4.2: the Variant column has an existence default there, so the scan falls back and Spark's reader returns the exact bytes.
  • The sketch scoping above, from @sunchao's review, replaces the three file-wide headers. Checked locally by running Spark's own SQLQueryTestSuite from the published spark-sql_2.13-4.2.0-tests.jar on Comet's -Pspark-4.2 classpath, pointed at a v4.2.0 tree with the diff applied and given the Spark SQL job's Comet settings (ENABLE_COMET_ONHEAP=true, and the SharedSparkSession confs as system properties). The published suite lacks the diff's harness patches, so the opt-out ran through a subclass.
    • With the opt-out, all three files match their golden files (6/6 including the analyzer variants), and 244 executed plans contained a Comet operator, 208 of them a Comet native scan.
    • Without it, exactly the 24 SELECTs over the two tables fail, all with FAILED_READ_FILE from encountered non UTF-8 data, and every other query matches.
  • At the pushed head, CometNativeScanSuite, CometScanRuleSuite, CometVariantTypeSuite, CometVariantProjectionSuite, CometVariantShreddingSuite, CometWindowExecSuite, CometRegExpJvmSuite, and CometSqlFileTestSuite for geospatial_types, window_filter and variant pass under -Pspark-4.2 and the default profile (172 tests each, spotless and scalastyle included). -Pspark-3.4 -Pscala-2.12 compiles main and test sources. cargo fmt and cargo clippy --all-targets -- -D warnings are clean. check-ci-config.py, check-suites.py, check-benchmark-runner.py and test-iceberg-shards.py pass. The diff applies with no rejects to a pristine v4.2.0 worktree and regenerates byte-identical.
  • Not run locally: the patched SQLQueryTestSuite itself, which needs an sbt build of Spark. run-spark-4.2-tests is still on this PR, so the Spark SQL Tests (Spark 4.2) shards on this push are the verdict, sql_core-2 in particular.

andygrove added 9 commits May 4, 2026 09:00
Adds dev/diffs/4.2.0-preview4.diff so the Spark SQL test suite can run
against Apache Spark 4.2.0-preview4 with Comet enabled, and wires the
4.2 profile into the spark_sql_test workflow matrix. The diff was
seeded from 4.1.1.diff and reconciled against v4.2.0-preview4.
Inherited from 4.1.1.diff during reconciliation. The Spark build was
trying to resolve comet-spark-spark4.1_2.13 instead of the 4.2 artifact,
causing the sql_hive jobs to fail before any tests ran.
Spark 4.2 removes GeographyVal / GeometryVal and replaces the getGeography
and getGeometry accessors on SpecializedGetters with a single getBinaryView
returning BinaryView. The shim therefore can no longer be shared between 4.1
and 4.2 out of the spark-4.1+ source root: the major, minor-plus and minor
shim directories are all added as compile source roots, so a spark-4.2 copy
would be a duplicate class rather than an override.

Move the trait into per-version spark-4.1 and spark-4.2 copies.
Point spark.version at the released 4.2.0 instead of 4.2.0-preview4.

Spark 4.2.0 bundles its own copy of
org.apache.datasketches.memory.internal.ResourceImpl inside spark-catalyst,
which collides with the transitive datasketches-memory jar and fails the
maven-enforcer BanDuplicateClasses rule, so ignore that class.

Keep the test-scope Jetty pin at 11.0.26. Spark 4.2.0 ships Jetty 12, but the
Iceberg REST catalog test helper needs jetty-servlet, which Jetty 12 replaced
with jetty-ee10-servlet.
Reseed the Spark SQL test overrides from 4.1.2.diff against the v4.2.0 tag and
drop the 4.2.0-preview4 diff.

The notable reconciliation is the test harness refactor: Spark 4.2 shrinks
SQLTestUtils to a deprecated empty alias and moves withSQLConf,
stripSparkFilter and the test() override into QueryTest, with
SharedSparkSession now extending QueryTest directly. Comet's hooks
(isCometEnabled, the IgnoreComet skip and the stripSparkFilter Comet cases)
move to QueryTest accordingly. isCometEnabled has to qualify
classic.SparkSession there, since a bare SparkSession in that package resolves
to the unified class.
Regenerated with dev/regenerate-golden-files.sh --spark-version 4.2.

The q2, q5, q54 (v1_4) and q5a (v2_7) goldens are pruned: their plans now match
the shared fallback under the released 4.2.0, so the divergences seen on
4.2.0-preview4 are gone.

q77a (v2_7) is added because Spark 4.2 plans a OneRowRelation into its Union
branches. Comet cannot convert that leaf, so the Unions fall back and the
aggregates above them lose their Comet partial producer. The approved plan
records the degraded plan; see apache#4949.
Add a spark_4_2 job to the ci.yml umbrella calling spark_sql_test_reusable.yml
with Spark 4.2.0 on JDK 17, plus the matching spark_4_2 filter in
compute-changes.py and output on the changes job. Gate it behind the
run-spark-4.2-tests label so it runs on pushes to main but stays off the
default PR path while 4.2 support is experimental.

Update the user guide for the released 4.2.0 and refresh the new-Spark-version
contributor guide, which still described the spark_sql_test.yml matrix that was
replaced by the per-version umbrella jobs.
@andygrove andygrove added the run-spark-4.2-tests Run the Spark 4.2 SQL tests on this PR label Jul 16, 2026
The preflight job only proceeds on a labeled event for known gating labels.
Without run-spark-4.2-tests in that allowlist, labelling a PR to request the
Spark 4.2 SQL tests skipped the whole pipeline instead of running them.
Comment thread .github/workflows/ci.yml Fixed
@andygrove
andygrove marked this pull request as draft July 17, 2026 15:57
Spark 4.2.0 is now in Maven Central, so the spark-4.2 profile no longer
needs to target a preview. Point spark.version at 4.2.0 and make the
main sources compile against it.

- Drop the WIP compile-only comment on the spark-4.2 profile.
- Spark 4.2.0 bundles its own copy of
  org.apache.datasketches.memory.internal.ResourceImpl inside
  spark-catalyst, colliding with the transitive datasketches-memory jar
  and failing the BanDuplicateClasses enforcer rule; ignore that class.
- Spark 4.2 removes GeographyVal / GeometryVal and replaces
  getGeography / getGeometry on SpecializedGetters with a single
  getBinaryView returning BinaryView. CometInternalRowShim can no longer
  be shared from the spark-4.1+ source root, so split it into per-version
  spark-4.1 and spark-4.2 copies.
- Correct the stale Jetty comment that referenced preview4.

This does not wire up the Spark SQL tests for 4.2; it only moves the
profile onto the released version and keeps it compiling.
Regenerated with dev/regenerate-golden-files.sh --spark-version 4.2
against the released 4.2.0. The q2, q5, q54 (v1_4) and q5a (v2_7)
goldens are pruned because their plans now match the shared fallback on
4.2.0, and q77a (v2_7) is added to record a degraded plan.
Two Spark 4.2.0 changes surfaced by moving the profile off preview4:

Iceberg scans job: Spark 4.2.0 turned `connector.catalog.View` from an
interface into a class. No Iceberg spark-runtime is published for 4.2, so
the build reuses the 4.0 runtime, whose `SparkView implements View` now
throws IncompatibleClassChangeError at class-load and aborts the Iceberg
suites. Report Iceberg as unavailable on Spark 4.2 in the shared probes so
the suites skip. Guard the fuzz suite's beforeAll with an early return
(cancelling from beforeAll aborts the suite) and add the missing
`assume(icebergAvailable)` to one native-scan test.

Expressions job: Spark 4.2 normalizes NaN / -0.0 for array_distinct and
the array set operations by wrapping their inputs as
`KnownFloatingPointNormalized(ArrayTransform(arr, x -> NormalizeNaNAndZero(x)))`.
The serde only handled a scalar `NormalizeNaNAndZero` child and fell back.
Since `KnownFloatingPointNormalized` is a runtime no-op tag, serialize any
other child directly and let its serde (the ArrayTransform codegen
dispatcher) carry the normalization, keeping these operations native.
# Conflicts:
#	spark/src/main/scala/org/apache/comet/serde/contraintExpressions.scala
CometNativeScan serializes the full data and partition schema, not just
the required columns. A table with a GEOMETRY or GEOGRAPHY column (Spark
4.2) has no proto representation for those types, so schema2Proto threw
NoSuchElementException during planning instead of falling back. Reject
the native scan when any data or partition schema field type cannot be
serialized.
…issues

Handle new-in-4.2 Spark SQL test suites that assert on Spark-internal
execution mechanics Comet replaces wholesale, and repoint deferred
assumes at specific issues instead of the closed generic tracker (apache#4142).

- SLAM metric suites, segment-tree window suites, and the UnionExec
  whole-stage-codegen test are marked IgnoreComet / IgnoreCometSuite
  (apache#4963, apache#4964, apache#4965).
- collect_set NaN/-0.0 normalization deferred (apache#4966).
- Repoint assume(!isSpark42Plus) guards to specific issues: apache#4967
  (ANSI arithmetic), apache#4968 (BloomFilter), apache#4969 (Iceberg REST catalog).
- Geometry/geography scan tests now pass via native-scan fallback.
@andygrove andygrove modified the milestone: 1.0.0 Jul 27, 2026
CodeQL flagged ci.yml as not limiting the GITHUB_TOKEN, so it inherits whatever
the repository default is. Adds a top-level `permissions: contents: read`,
matching the convention already used by codeql.yml and pyarrow_udf_test.yml.

A blanket read-only default is not quite safe here, because ci.yml calls six
reusable workflows and a callee inherits the caller's permissions and can only
narrow them further. Audited all six: docs.yaml commits and pushes the generated
site to the asf-site branch, so it needs write; the other five (pr_build_linux,
pr_build_macos, pr_benchmark_check, spark_sql_test_reusable,
iceberg_spark_test_reusable) only build and test. So the `docs` job raises itself
to `contents: write` and nothing else changes. That job already only runs on
push-to-main or workflow_dispatch, never on pull requests.

Also checked what else might quietly depend on write access: no job in ci.yml or
in any callee uses GITHUB_TOKEN, secrets, the gh CLI, git push (other than
docs.yaml), containers or packages. The upload/download-artifact steps are all
same-run, which uses the Actions runtime token rather than GITHUB_TOKEN, and none
pass run-id or github-token, so none need `actions: read`.

Verified the file parses and the blocks land where intended (top-level
contents: read, docs job contents: write, no other job overriding). actionlint
could not be run locally -- its installer is a curl-to-shell script -- so it will
be exercised by the preflight job on this PR.
`PR Build [expressions]` (Spark 4.0 / 4.1 / 4.2) failed on
`expressions/misc/variant.sql`: the new full-data-schema serializability
check in `CometNativeScan.isSupported` ran before the projected-schema type
check, so a `struct<v: variant>` column was reported as "Native scan does not
support data type struct<v:variant>" instead of the expected "type
VariantType". Run the schema-support check first so the more specific type
reason wins; the geospatial case that motivated the check still falls back
(and no longer crashes planning), now covered by a new
`expressions/misc/geospatial_types.sql` fixture.

`Spark SQL Tests (Spark 4.2)` sql_core failures:

- Spark 4.2 allows `FILTER (WHERE ...)` on window aggregates. Comet dropped
  the filter (it is only serialized for Partial mode aggregates, and window
  aggregates are Complete mode) and returned wrong results for `window.sql`.
  Decline these window expressions in `CometWindowExec`, and turn the silent
  drop in `aggExprToProto` into a fallback for any non-Partial mode carrying a
  filter. Covered by `expressions/window/window_filter.sql`.
- `dev/diffs/4.2.0.diff` was reseeded from `4.1.2.diff` and missed the updates
  main has since made to the 4.1 diff, which caused the `SubquerySuite`,
  `CachedBatchSerializerNoUnwrapSuite` and `DataFrameAggregateSuite`
  (x4 suites) failures. Ported: the `CometHashAggregateExec` arm for
  SPARK-22223, the `WholeStageCodegenExec(CometColumnarToRowExec(...))` scan
  shapes for SPARK-26893, the codegen-aware cached-plan transition check, the
  `VariantEndToEndSuite` patch, `spark.comet.shuffle.enabled` for the renamed
  config, and `comet.version` 1.1.0-SNAPSHOT.
- `tuplesketch.sql` hits the same collated-string sketch problem as
  `thetasketch.sql`, so it joins the ignore list. `join-nearest-by.sql` records
  Spark operator names in EXPLAIN goldens and `in-order-by.sql` orders by a
  column with ties, so both run with Comet disabled (as explain*.sql and
  in-limit.sql already do).

Also merges apache/main, which the branch needed for the diff sync.
@andygrove
andygrove marked this pull request as ready for review August 5, 2026 18:04
@andygrove
andygrove requested a review from sunchao August 30, 2026 14:40
@andygrove andygrove removed the run-spark-4.2-tests Run the Spark 4.2 SQL tests on this PR label Sep 6, 2026
@andygrove

Copy link
Copy Markdown
Member Author

@sunchao @comphead I think the CI question was the main thing holding this up, so I've changed the gating: the new spark_4_2 job is now gated on workflow_dispatch only, so the Spark 4.2 SQL tests run neither on PRs nor on merges to main. They are on demand for now (Actions -> CI -> Run workflow). That also relates to @comphead's question above about whether we should stop running CI for 4.1 — I'd rather not add a whole extra Spark SQL matrix to every merge until we've decided which older version to retire in exchange, and enabling 4.2 on PRs can then be its own small PR.

For the record, the full 4.2 SQL run was green on this branch before I flipped the gate: all seven shards (catalyst, sql_core-1/2/3, sql_hive-1/2/3) passed at 65d16df. Nothing else in the PR changed, apart from the docs that claimed the SQL tests now run in CI. Could you take another look?

# Conflicts:
#	spark/src/main/scala/org/apache/comet/serde/operator/CometNativeScan.scala
@andygrove andygrove added this to the 1.1.0 milestone Sep 15, 2026
Four conflicts, all in the CI tiering that landed on main while this branch
was out (apache#5926, apache#5939, apache#5963):

- dev/ci/compute-changes.py: keep both the spark_4_2 FILTERS entry and main's
  spark_4_1_hive entry, and give spark_4_2 the POLICY entry it never had.
  compute() iterates FILTERS and calls event_allows(), which indexes POLICY,
  so a FILTERS key with no POLICY entry made the changes job raise
  KeyError: 'spark_4_2' on every event that is not a dispatch. The 4.2 filter
  list also picks up the inputs spark_4_1 gained since (spark-sql-modules.py,
  the artifact retry actions, .mvn and mvnw) so the shared-build routing cases
  cover it.
- .github/workflows/ci.yml: expose both new outputs; take main's Detect
  changes script, whose schedule case replaces the dispatch key list; keep
  main's docs if: but keep this branch's permissions: contents: write, which
  the new top-level contents: read default makes load-bearing for the asf-site
  deploy. The spark_4_2 job's event check moves into POLICY, and the job joins
  required_checks.needs so it cannot fail without blocking the queue.
- dev/ci/check-ci-config.py: declare spark_4_2 in BUILD_JOBS, in a
  SPARK_EXPERIMENTAL tier set feeding ALL_JOBS, and in POLICY_CASES.
- .github/workflows/README.md: take main's tables and add the 4.2 rows.

Spark 4.2 keeps its on demand only routing, now expressed as
label:run-spark-4.2-tests (the label already exists on the repo) rather than
an event check in ci.yml. adding_a_new_spark_version.md described the old
dispatch gating, a key list main deleted, and a preflight label allowlist that
no longer exists, so its CI section is rewritten to match.

check-ci-config.py, actionlint and prettier all pass.
@github-actions github-actions Bot added area:scan Parquet scan / data reading area:expressions Expression evaluation area:Iceberg labels Sep 16, 2026
Comment thread dev/diffs/4.2.0.diff Outdated
@rich7420

Copy link
Copy Markdown
Contributor

@andygrove thanks for the patch and patience

apache#5881 added `.github/actions/maven-bootstrap/**` to every Spark SQL job's
change filter. `spark_4_2` exists only on this branch, so that commit could
not reach it, and merging main produced no conflict: the entry was appended
to five sibling lists and the sixth simply never grew one.

check-ci-config.py catches this -- `spark_4_2` is in BUILD_JOBS, so the
maven-bootstrap routing case expects it -- and it fails on the merge commit
as it stands, which would take preflight down with it.
4.2.0.diff was seeded from 4.1.3.diff and last reconciled against main on
2026-08-31. Seven commits have changed 4.1.3.diff since, and none of them
could reach 4.2.0.diff: the file exists only on this branch, so every one of
them merged cleanly while leaving the 4.2 diff on the old behaviour.

Ported, each regenerated from a v4.2.0 checkout rather than hand-edited:

- apache#5755, apache#5760, apache#5745, apache#6046 restored coverage that Comet had since fixed.
  4.2.0.diff still carried the skips: the recursive HAVING/ORDER BY fixture,
  the DELTA_LENGTH_BYTE_ARRAY test and `withAllParquetWriters`, the two
  Variant shredding suites, and the invalid-row-index-column `assume(false)`.
- apache#5914 added `--SET spark.comet.enabled=false` to two ORDER BY fixtures
  whose tied rows have no deterministic order.
- apache#5987 mixed IgnoreCometSuite into the RocksDB state-store suites. Five were
  missing entirely; RocksDBStateStoreIntegrationSuite had the mixin last,
  which skips only the plain `test(...)` registrations, so it moves ahead of
  AlsoTestWithRocksDBFeatures like the others.
- apache#5821 replaced the direct `isInstanceOf[EmptyRelationExec]` assertions in
  AdaptiveQueryExecSuite with an `isEmptyRelation` helper that also accepts
  CometEmptyRelationExec.

Two 4.1.3 hunks are deliberately not ported. AdaptiveQueryExecSuite weakens
`localReads.length` to 1 inside two tests that 4.2 already annotates with
IgnoreComet, so on 4.2 the assertion is unreachable either way; leaving
Spark's own value there keeps the diff smaller.

Verified: applies with no rejects to a pristine v4.2.0 worktree, and every
file the two diffs share now carries the same Comet annotations apart from
the documented 4.2 reconciliations (the SQLTestUtils-to-QueryTest move, the
extra BroadcastHashJoinExec parameter, and the 4.2-only sketch fixtures).
The 4.2 suite was left on demand -- the `run-spark-4.2-tests` label or a
dispatch -- on the grounds that a brand new version should not gate anything
until it settles. It still gates nothing in the nightly tier: a red nightly
opens a `ci-nightly-failure` issue and blocks no merge.

What on demand actually cost is the 4.2 diff file. Every other version's diff
is updated as a side effect of its suite running; 4.2's was updated only when
someone asked for the suite, and it spent three weeks silently behind the
others. Nightly is what keeps it current.

`spark_4_2` moves from its own SPARK_EXPERIMENTAL set into NIGHTLY_TIER,
which is also what makes the `schedule` POLICY case assert its presence
rather than its absence. The label keeps working and now means the same thing
it means for 3.5 and 4.0: bring the nightly run forward onto this pull
request.
@andygrove andygrove added the run-spark-4.2-tests Run the Spark 4.2 SQL tests on this PR label Sep 22, 2026
…ders

apache#5914 replaced the `replacing-missing-expression-with-alias.sql` and
`in-set-operations.sql` entries in `SQLQueryTestSuite.ignoreList` with
`--SET spark.comet.enabled=false` headers in the fixtures themselves, and
closed apache#5570. The previous commit carried the headers into 4.2 but left the
`ignoreList` entries in place, which changes nothing: `createScalaTestCase`
consults `ignoreList` before any Comet-specific dispatch, so both files were
still registered as ScalaTest `ignore` and skipped in the `ENABLE_COMET=false`
baseline too.

Removing them leaves 4.2 with no Comet additions to `ignoreList` at all --
the sketch files already moved to fixture headers in e0c7963 -- so every
`.sql` fixture now runs in the baseline, and the two ordering-sensitive ones
run with Comet disabled rather than not running.
apache#6085 rewrote the tier section of ci.md around a mermaid diagram and a
suite-by-suite table, both of which landed while this branch had 4.2 on
demand. Adds 4.2 to the nightly row of each, and `run-spark-4.2-tests` to the
opt-in label table.
@andygrove

Copy link
Copy Markdown
Member Author

Updated: merged current main, addressed the outstanding review feedback, and moved the 4.2 suite into the nightly tier.

Nightly instead of on demand. @comphead / @sunchao, this reverses what the PR previously said. Leaving 4.2 on demand looked like the cautious choice, but the caution was aimed at the wrong risk. The tier a version sits in is not only a compute decision: a dev/diffs file is kept current as a side effect of its suite running, and 4.2's was updated only when someone asked for the suite. It still gates nothing -- a red nightly opens a ci-nightly-failure issue and blocks no merge -- and run-spark-4.2-tests still brings the run forward onto a pull request.

What the merge actually needed. The merge was conflict-free and still wrong in two places, neither of which git can point at, because both are cases where this branch owns a thing that upstream commits extend on the sibling entries:

That resync closes the three items I had deferred to #5569, which have since been fixed on main for 4.0/4.1, plus @rich7420's two points. Replies are on the individual threads.

Net effect on exclusions. ignoreList in the 4.2 patch now has no Comet additions at all, so every .sql fixture runs in the ENABLE_COMET=false baseline. That is one better than 4.1.3.diff, which still lists thetasketch.sql (#5571).

Verified. -Pspark-4.2 and the default profile both build main and test sources clean, scalastyle included; the diff applies with no rejects to a pristine v4.2.0 worktree; check-ci-config.py, check-suites.py, check-benchmark-runner.py, test-iceberg-shards.py, actionlint and prettier --check all pass. Spark's own 4.2 suite I cannot run locally, so I have applied run-spark-4.2-tests -- the Spark SQL Tests (Spark 4.2) shards on this PR are the verdict on the resynced diff.

DataFusion's parquet RowFilter flattens a predicate evaluation error to a
string, so a Java exception raised by a codegen-dispatched expression in a
pushed-down filter lost its type and surfaced as a CometNativeException.
Spark 4.2's st-functions.sql hits this with an invalid ST_AsBinary
endianness in a WHERE clause when row-level pushdown is enabled.

Drop filters containing a JvmScalarUdfExpr before pushing data filters
into the scan. The Filter above the scan still evaluates them, and a JVM
call cannot drive row-group, page-index, or bloom-filter pruning anyway.
# Conflicts:
#	spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala
@andygrove

Copy link
Copy Markdown
Member Author

@sunchao @comphead could I get another review?

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

  • Prior state and problem: Spark 4.2 SQL tests were not scheduled. Enabling them exposed unsupported-schema planning failures, ignored window filters, and lost JVM exception types.
  • Design approach: Add the Spark 4.2 patch and nightly CI routing, with targeted runtime guards and regression tests.
  • Correctness / compatibility analysis: The guards match the relevant Spark semantics checked across 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. No additional introduced P1/P2 issues found within this review.
  • Key design decisions: Reusing nativeDataSchema keeps validation and serialization consistent. Reusing liftFallbackReasons preserves explanations for rewritten decimal windows without another abstraction.
  • Implementation sketch: Reject unserializable scan schemas and filtered window aggregates, propagate fallback reasons, and exclude JVM predicates from Parquet row filtering while retaining their evaluation above the scan.
  • Behavioral changes worth calling out: Spark 4.2 runs nightly or through its PR label. The runtime changes add schema/expression traversals during planning and scan initialization. No performance benchmark was run.
  • Suggested improvements: The previously reported P2 sketch-coverage concern remains present despite the resolved thread. dev/diffs/4.2.0.diff:372 and :385 disable Comet for entire sketch fixtures. Spark's getSparkSettings and runSqlTestCase apply those settings to every query, including hundreds unrelated to invalid UTF-8. Narrowing these exclusions remains open in #5571. This is tracked but not fixed, so existing_blockers is true. No duplicate inline finding is included.

Reviewed the entire 23-file diff from 5d59317630e19d22cc9be5dbdae59ae1b87b4f41 to 248e59c3963204b50dd3fbcb98fa92470ae0ac7b. The PR remained non-draft. Read existing reviews, discussion, and all 32 inline comments. Routed skills: review-comet-pr, review-comet-expression-pr, and review-comet-ffi-pr.

Exact-head CI: 31 checks passed, 15 skipped, none failed or pending. All seven Spark 4.2 SQL shards passed, alongside Rust tests and the default Spark 4.1 suites. The tested merge commit has the same tree as the reviewed head. Logs confirm the new nested-JVM-predicate unit test and JVM-filter exception regression passed.

Validation limits: Locally, CI configuration checks passed, and the patch applied cleanly to official Spark v4.2.0; the patched Spark tree passed git diff --check. Rust/JVM/Spark suites were not rebuilt or rerun locally. Runtime evidence comes from CI. The two new SQL fixtures are version-gated and skipped in the default Spark 4.1 run. No project code or GitHub state was changed.

# Conflicts:
#	spark/src/main/scala/org/apache/comet/serde/operator/CometNativeScan.scala
apache#6066 removed memory accounting from Comet's on-heap mode, which the Spark SQL
jobs run in, and dropped this setting from the other version diffs. 4.2.0.diff
only exists on this branch, so the merge left it behind.
…alid UTF-8

hll.sql, thetasketch.sql and tuplesketch.sql each ran every query with Comet
disabled because one table in each file holds invalid UTF-8, which Comet's
native scan rejects (apache#4121). Only the eight queries per file that read that
table fail, so the rest of each file lost Comet coverage for nothing.

SQLQueryTestSuite now disables only the native scan, and only for queries that
name hll_string_test or t_string_collation, and the three fixture headers are
gone. Doing it in the harness rather than with SET statements in the fixtures
keeps the diff free of golden-file changes.
@andygrove

Copy link
Copy Markdown
Member Author

Pushed 7c8e995. @sunchao, the sketch coverage you flagged is now scoped: only the 24 queries that read invalid UTF-8 run without the native scan, and the rest of hll.sql, thetasketch.sql and tuplesketch.sql runs with Comet. Details are on the original thread.

The rest of this update:

run-spark-4.2-tests is still applied, so the Spark 4.2 SQL shards on this push are the check on the patched harness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:expressions Expression evaluation area:Iceberg area:scan Parquet scan / data reading build Build environment enhancement New feature or request run-spark-4.2-tests Run the Spark 4.2 SQL tests on this PR spark 4.2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants